-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Generate types for all bindings across all environments #11893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 9a5d3c6 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
vicb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick first pass, added some comment.
I really like the added comments 🙏
dario-piotrowicz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments as an initial first review, please have a look 🙂🙏
| MY_VAR?: \\"a var\\"; | ||
| MY_VAR_A: \\"A (prod)\\" | \\"A (stag)\\" | \\"A (dev)\\"; | ||
| MY_VAR_B?: {\\"value\\":\\"B (prod)\\"} | {\\"value\\":\\"B (dev)\\"}; | ||
| MY_VAR_C?: [1,2,3] | [\\"a\\",\\"b\\",\\"c\\"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if this would constitute a breaking change or a bug fix? Since technically it's sort of both?
Previously Env set variables to be a required union, regardless whether the variable existed across all environments or not. This has now been changed such that variables in Env are still a union, but are now optional unless that variable exists across all named environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mh.... the same can be said to any other binding no?
For example I could have something like:
"kv_namespaces": [{
"binding": "MY_KV",
"id": "xxx"
}],
"env": {
"my_env": {
"kv_namespaces": []
}
}with the previous iteration of wrangler you'd get MY_KV: KVNamespace while now you'd get MY_KV?: KVNamespace (since MY_KV is not declared for the my_env environment) no? (so here again users could go from a normal variable before to an optional one after)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification, yeah it's pretty much as I thought...
I agree that there's the risk of breaking folks... I personally would think that this is ok since this makes the types more accurate (so it could be considered a bugfix?) but you can ask for a second opinion from the team someone might think of this differently (we could make this opt-in if we were really concerned 🤔)
dario-piotrowicz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good to me, sorry but I still need to give this another review (or 2) before feeling somewhere ok approving it.
I also need to run the functionality locally.
Also given the size of the PR I think it would be appropriate if there are at least a couple of approvals here before we merge this PR


Fixes #10400, #9709 and #8475
Currently, if you run
wrangler typesit only generates TypeScript types for bindings (KVNamespace,D1Database, etc) defined in the top-level configuration (or a single environment when using--env). Now, by default, it collects & generates types for bindings from all environments in your configuration.This ensures your generated types include all bindings that might be used across different deployment environments (e.g., staging, production), preventing TypeScript errors when accessing environment-specific bindings.
Additionally, If the same binding name exists with different types across environments (e.g.,
CACHEis a KV namespace in one environment but an R2 bucket in another), an error will be thrown to prevent type conflicts.A picture of a cute animal (not mandatory, but encouraged)